New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enforce that symbol comparison should use an equality comparer #2764
Enforce that symbol comparison should use an equality comparer #2764
Conversation
❓ Have you verified that this diagnostic does not suggest using an EqualityComparer if the user is building against Roslyn 1.x or 2.x? For those releases, |
I have not yet. I also may have misinterpreted the recommendation from @jasonmalinowski . Is there a good way to version lock? Can we check for the existence of the wrapped type? |
Is there a specific equality comparer type in Roslyn that was added in 3.x? If so, you can fetch this named type symbol at compilation start and keep your logic guarded around the presence of that type symbol. |
private const string s_symbolEqualityComparerName = typeof(SymbolEqualityComparer).FullName;
...
comparerType = compilation.GetTypeByMetadataName(s_symbolEqualityComparerName); Is that the correct way? Currently it doesn't look like the version of Roslyn being referenced by the analyzers is up to date enough to have the comparer. |
@ryzngard Master is currently targeting 2.9.0 version of MS.CA so that the analyzers can execute on Dev15 toolset. You may want to consider retargeting your PR to 3.x branch and move the version of referenced MS.CA in that branch to your required Roslyn version. You would need to change the following: roslyn-analyzers/eng/Versions.props Lines 21 to 22 in ddb105b
roslyn-analyzers/eng/Versions.props Line 5 in ddb105b
|
Actually, stepping back. If you don't need to invoke any new Roslyn API in 3.x version and are just looking for a type symbol for |
src/Microsoft.CodeAnalysis.Analyzers/Core/CodeAnalysisDiagnosticsResources.resx
Outdated
Show resolved
Hide resolved
@sharwell with the updated code I'm having trouble with tests. Is there an assembly reference I should add? I would have thought
|
I believe the tests would also reference the older (2.9.x) version of code analysis. You should extend your test code to define the required equality comparer in test source code with stub implementations, we do so in many tests. For example, see https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.Analyzers/UnitTests/UpgradeMSBuildWorkspaceAnalyzerTests.cs#L20 |
@mavasani ah, thanks. For some reason I thought the public comparer was older but was added at the same time. I'll update the test |
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.Analyzers/UnitTests/MetaAnalyzers/CompareSymbolsCorrectlyTests.cs
Show resolved
Hide resolved
src/Microsoft.CodeAnalysis.Analyzers/Core/MetaAnalyzers/CompareSymbolsCorrectlyAnalyzer.cs
Outdated
Show resolved
Hide resolved
return; | ||
} | ||
|
||
var parameters = invocationOperation.Arguments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: rename to arguments
With the introduction of nullable reference types, comparison of symbols has become more complex. The recommended way is to use an EqualityComparer. This adds a check for the
Equals
method where all parameters areISymbol
types. If the method invocation is from an instance, the instance is also checked to beISymbol
.Also updated the text to reflect new recommended comparison and resolution.